Skip to content

feat(config-ui): added prettier to help format code#2995

Merged
klesh merged 1 commit into
apache:mainfrom
mintsweet:feat-prettier
Sep 20, 2022
Merged

feat(config-ui): added prettier to help format code#2995
klesh merged 1 commit into
apache:mainfrom
mintsweet:feat-prettier

Conversation

@mintsweet
Copy link
Copy Markdown
Member

⚠️ Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation.
  • I have added relevant tests.
  • I have added relevant documentation.
  • I will add labels to the PR, such as pr-type/bug-fix, pr-type/feature-development, etc.

Summary

Does this close any open issues?

No.

Screenshots

Not any.

Other Information

Ensure the consistency of multi-person collaborative code style.

@e2corporation
Copy link
Copy Markdown
Contributor

@mintsweet Prettier will cause conflicts with ESLint, especially with regard to single quotes use and JSX files. There is a method to have ESLint working together with prettier but it requires more configuration. When Prettier auto-formats JSX it will replace all single quotes with double quotes, ignoring the single quote rule altogether making it a requirement to run eslint again to backfix the double quotes.

While prettier can be used locally for now, I don't want prettier formally checked into the codebase auto-formatting files until it has been properly tested & configured to operate with ESLint & JSX.

Comment thread config-ui/.prettierrc.js Outdated
@e2corporation
Copy link
Copy Markdown
Contributor

My current local settings for Prettier+ESLint, I may increase 80 to 120 as far as print width goes.

Prettier RC

{
  "trailingComma": "es5",
  "tabWidth": 2,
  "printWidth": 80,
  "semi": false,
  "singleQuote": true
  }

ESLint RC

{
  "extends": ["prettier"],
  "plugins": ["prettier"],
  "rules": {
    "prettier/prettier": ["error"],
    "jsx-quotes": ["error", "prefer-single"],
  }
}

@e2corporation e2corporation marked this pull request as draft September 7, 2022 13:34
@mintsweet
Copy link
Copy Markdown
Member Author

@e2corporation Prettier has a property jsxSingleQuote that fixes the problem you said. Everyone's code formatting style is different, using prettier is to ensure that the style is consistent under the same repository. I also added eslint-prettier.

@mintsweet
Copy link
Copy Markdown
Member Author

@e2corporation I observed that the max-len configured by eslint is 140, so I also set the printWidth of the prettier to 140.

@mintsweet mintsweet marked this pull request as ready for review September 8, 2022 05:14
@e2corporation
Copy link
Copy Markdown
Contributor

@mintsweet The jsxSingleQuote is partially deprecated and even though it is set to true it will be ignored in most cases. This setup needs to be properly validated to ensure proper behavior before merging this PR to the codebase. You can run prettier+eslint locally for now against new files you create, prettier should not be run against all files in the codebase at this time. Global code formatting enhancements can be done in the future under another PR for tech debt.

@e2corporation
Copy link
Copy Markdown
Contributor

@mintsweet This PR can be planned for the next upcoming milestone, not v0.14.0.

@e2corporation
Copy link
Copy Markdown
Contributor

@mintsweet Our UI stack also prefers no-semicolons. Please adjust your local settings accordingly so semicolons are not added to JS or JSX Files

@mintsweet
Copy link
Copy Markdown
Member Author

@e2corporation I added semi: false to the prettier config file to make sure there are no semicolons in the file. And I hope this PR can be merged as soon as possible. At present, I can't use formatting directly or I will use my previous code style, but I need to use the previous code style in other repositories. Hope to get your understanding.

@e2corporation
Copy link
Copy Markdown
Contributor

@mintsweet What is your primary Editor/IDE? Your Editor should be configured so configurations for Prettier and ESLint are read from the devlake project's config-ui directory, this way your global editor settings or other project specific settings are unaffected. These configuration files can exist on your local for now without checking them into the codebase and that should still allow you the ability to format.

I have to complete other milestone tasks before shifting focus to properly review this ticket and test locally before merging.

@mintsweet
Copy link
Copy Markdown
Member Author

@e2corporation vscode.
OK, do things with higher priority first.

@klesh
Copy link
Copy Markdown
Contributor

klesh commented Sep 19, 2022

According to what I learned from @mintsweet, the configuration of this PR is compatible with the current codebase.

The plan is:

  1. Setup prettier based on the current codebase in this milestone
  2. Maybe change some settings as we see fit, Must format all config-ui source files in the next milestone

If I understand it correctly, this PR should be merged ASAP:

  1. This is local-setup, specifically for config-ui in terms of developers working on multiple frontend projects at the same time.
  2. This is not a breaking-change at the moment
  3. This is making @mintsweet hard to do his work, because he has to format the code manually. Asking one to set it up on IDE/Editor level(Globally) is not a good way for an open source project.

@e2corporation I think we should merge this one first, what do you think?

@e2corporation
Copy link
Copy Markdown
Contributor

e2corporation commented Sep 19, 2022

@klesh While it's good to have prettier configuration added to make thing easier, the point I would like to make is that the Developer is responsible for following the project's coding standards. Having these files checked in should not be needed or should not be preventing @mintsweet from completing his work on Webhooks.

These are the minimal coding requirements to work on DevLake's UI, regardless of what Editor/IDE is being used. Aside from these basic rules, ESLint will ensure all files are meeting current spec. (This should be added to the UI readme)

  • 2 Tab Spaces
  • Convert Tab to Spaces TRUE
  • Prefer Single Quotes
  • Trailing Commas: ES5
  • Column Width: 140

This PR also adds prettier to npm scripts that could affect all files on the UI, which we don't want right now because that will make diffing code changes problematic. Developers should be able to format JSX code without always having to rely on an auto-formatter. Why were semi-colons and double-quotes added to these new project files in the first place when none of our other codebase files contained them?

Comment thread config-ui/package.json
@e2corporation
Copy link
Copy Markdown
Contributor

e2corporation commented Sep 19, 2022

@klesh @mintsweet This PR needs a rebase. PR Description also needs an update.

Copy link
Copy Markdown
Contributor

@e2corporation e2corporation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (after rebase)

@e2corporation e2corporation added the component/config-ui This issue or PR relates to config-ui label Sep 19, 2022
@e2corporation e2corporation added this to the v0.14.0 milestone Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/config-ui This issue or PR relates to config-ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants